Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UAVCAN v1.0 done #142

Merged
merged 100 commits into from
Mar 8, 2020
Merged

UAVCAN v1.0 done #142

merged 100 commits into from
Mar 8, 2020

Conversation

pavel-kirienko
Copy link
Member

The library has been somewhat reworked. Don't look at the diff, it is too large -- this change is impossible to do incrementally in a sensible way.

The new implementation offers hard time complexity guarantees as discussed in OpenCyphal-Garage/libcyphal#185. The new implementation supports both Classic CAN and CAN FD without the need for compile-time configuration selection (#103). The new implementation offers a much simplified API that operates on contiguous memory buffers instead of small fixed-size blocks (https://forum.uavcan.org/t/canardrxtransfer-payload-reading/600).

After this PR is merged, I am going to do the branch swap: master will be v1.0, and the current master will become legacy-v0.

Fixes #131
Fixes #129 (this case is covered by tests)
Fixes #126
Fixes #125
Fixes #117 (completely new memory management)
Fixes #105
Fixes #104 (there is no DESIGN.md, the description has been moved into the header file)
Fixes #102 (100% test coverage)
Fixes #69 (https://forum.uavcan.org/t/platform-specific-components/768)
Closes #132

@pavel-kirienko pavel-kirienko added this to the v1.0 milestone Mar 3, 2020
@pavel-kirienko pavel-kirienko self-assigned this Mar 3, 2020
@Linjieqiang
Copy link
Contributor

Great News!

@LorenzMeier
Copy link

Codecov Report

Merging #142 into uavcan-v1.0 will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           uavcan-v1.0     #142   +/-   ##
============================================
  Coverage        46.76%   46.76%           
============================================
  Files               13       13           
  Lines             4965     4965           
============================================
  Hits              2322     2322           
  Misses            2643     2643           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5f1b77...b5f1b77. Read the comment docs.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (uavcan-v1.0@c1cf930). Click here to learn what that means.
The diff coverage is 96.67%.

Impacted file tree graph

@@              Coverage Diff               @@
##             uavcan-v1.0     #142   +/-   ##
==============================================
  Coverage               ?   46.76%           
==============================================
  Files                  ?       13           
  Lines                  ?     4965           
  Branches               ?        0           
==============================================
  Hits                   ?     2322           
  Misses                 ?     2643           
  Partials               ?        0
Impacted Files Coverage Δ
libcanard/canard.c 100% <ø> (ø)
tests/test_self.cpp 100% <100%> (ø)
tests/exposed.hpp 100% <100%> (ø)
tests/test_private_rx.cpp 100% <100%> (ø)
tests/test_private_crc.cpp 100% <100%> (ø)
libcanard/canard_dsdl.c 100% <100%> (ø)
tests/test_public_tx.cpp 100% <100%> (ø)
tests/test_dsdl.cpp 100% <100%> (ø)
tests/test_public_rx.cpp 100% <100%> (ø)
tests/test_private_tx.cpp 100% <100%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1cf930...b5f1b77. Read the comment docs.

@pavel-kirienko
Copy link
Member Author

Disabled the codecov integration to prevent confusion. SonarQube is already set up to track the coverage correctly.

Copy link
Member

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial feedback.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
libcanard/canard.h Show resolved Hide resolved
README.md Show resolved Hide resolved
libcanard/canard.h Outdated Show resolved Hide resolved
libcanard/canard.h Outdated Show resolved Hide resolved
libcanard/canard.h Show resolved Hide resolved
libcanard/canard.h Show resolved Hide resolved
libcanard/canard.h Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@thirtytwobits
Copy link
Member

So, I can approve this based on a cursory reading of the code but I'm not saying I did a "good" code review because it's big. If you want a real code review it'll take longer than this weekend to go though and compile this myself, use it, and dive into the implementation.

State that the frame payload pointer may be NULL if the size is zero.
@pavel-kirienko
Copy link
Member Author

That's understandable. I think we can afford some laxity until Libcanard v1.0 is released (the current version is 0.100); I suppose we will have a few months at least to get major API-breaking changes in, shall the need arise. For now, seeing as there don't seem to be any major issues, I would like to have it merged and the branches swapped.

@sonarcloud
Copy link

sonarcloud bot commented Mar 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants